-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sanity_checks: accept dash in packages' version strings #1706
base: master
Are you sure you want to change the base?
sanity_checks: accept dash in packages' version strings #1706
Conversation
0efcfbf
to
b07d1cf
Compare
typo in commit message... |
Many software packaging systems handle this by fully rejecting such version strings and mandating that adding those packages to the database should first involve mangling the version string to a consistent format, for example replacing dashes with underscores. |
With this change, I believe the script would decide that the package version was |
with version equal to |
I'm not sure to understand... The documentation of wrapDB does have any constraint for the entries' version string. The only one is for the wrap filename (the given regexp is And if you look at the content of So, I don't see any reason for not supporting the dash in packages' version. |
It's not true that there's no restriction on dashes in the version number. The restriction is explicitly enforced by the code you're modifying. You're right that 1 |
I said the documentation does not give any restriction on the version, not there is no restriction at all. I said too that in the page Adding new projects to WrapDB, section Choosing the wrap name, the restriction on wrap names is non-sense. Here is the text: NOTE: Wrap names must fully match this regexp: [a-z0-9._]+ The contributor is also invited to read the WrapDB's README.md for the next step: Add your release information in releases.json. It is a dictionary where the key is the wrap name and the value is a dictionary containing with the following keys The first project in So, please, update the documentation.
Sure, it can, but where? In wrapDB? This is the purpose of this PR? In Meson? the function So, WrapDB should do the same.
Why are we debating that? WrapDB use wrap files to avoid such headache.
Thanks, I was too hasty... I reworked my PR (and I accept any complain about the function name).
I fixed the test name too. scripts But, the regexp I intend to change accepts underscores. So, how can There is no project with such version, so the error never occurs. And $ ./tools/create_release.py
...
requests.exceptions.InvalidSchema: No connection adapters were found for 'ftp://ftp.gnu.org/gnu/gdbm/gdbm-1.23.tar.gz'
... Is this script still used? The script ...
$ ./tools/import-wraps.py whatever
Traceback (most recent call last):
File "/home/..././tools/import-wraps.py", line 162, in <module>
all_wraps.remove('sqlite')
ValueError: list.remove(x): x not in list
... Is this script still used? Do I open new issues for those 2 crashes or do I let you investigate on your side? |
b07d1cf
to
53edee2
Compare
It is used on every single merge to master. It also uses requests instead of urllib, which is why such a wrap works fine with meson but not with the create_release.py script... something that matters on the irregular occasion that the single wrap using an ftp url (gdbm) gets updated. It hasn't happened in years, since the code in create_release.py that tries to use it was added. ;)
That script was implemented for a one-time event, and removing wraps that have been deleted will no longer work now that they are deleted and no longer in the list! :) It is not intended to be still used, it is there for historicity. |
@eli-schwartz: thanks for the full explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dashes in versions can still cause problems, though, even if the WrapDB tooling were okay with it.
Sure, it can, but where?
Downstream. These sorts of changes can break third-party tooling. Maybe there aren't any tools that care, but it's worth at least considering whether reneging on our prior guarantees is worth doing.
Why should we not instead require wraps to convert dashes in the version string to underscores?
tools/utils.py
Outdated
@@ -128,3 +128,6 @@ def is_msys() -> bool: | |||
|
|||
def is_macos(): | |||
return any(platform.mac_ver()[0]) | |||
|
|||
def split_release_version(version: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: split_version_release
would be clearer because that's the order of the returned list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I follow your logic, split_version_revision
should be a little bit better, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes.
The elephant in the room is that the only reason dashes in the version are an issue here, is because of a proposed wrap of a beta release of a project. Does taocpp-json not consider itself to be broadly usable, that it should be 14 attempts deep into the beta release cycle and still no stable version in sight? |
The real concern is that meson accepts such version strings while With the next project: meson.build:
bar.pc
and in a shell: $ PKG_CONFIG_PATH=`pwd` meson setup . b
...
Dependency bar found: NO. Found 1.0.0-beta.13 but need: '>1.0.0-beta.14'
... It fails as expected. Using the comparison But using the comparison And the second important point is that the actual regexp accepts underscores in upstream version while it should reject them:
Because they both use |
upstream versions with underscores break the way wrap files are handled: this character is the delimiter used to extract the wrap's name and version from different sources: * `meson wrap` when retrieving patch files; * sanity_checks.py when retrieving git tags.
53edee2
to
56fbd77
Compare
Projects may use dashs ('-') in their version strings (for example, taocpp-json use "-beta.N" for years) but it prevents accepting such projecs in the wrapdb database.
As the string is splitted with rsplit(1), such version strings should not cause a problem.